Skip to content

Fix incorrect rounding with subnormal/zero results of float multiplication#637

Merged
Amanieu merged 3 commits into
rust-lang:masterfrom
beetrees:fix-float-mul
Jul 5, 2024
Merged

Fix incorrect rounding with subnormal/zero results of float multiplication#637
Amanieu merged 3 commits into
rust-lang:masterfrom
beetrees:fix-float-mul

Conversation

@beetrees
Copy link
Copy Markdown
Contributor

@beetrees beetrees commented Jun 30, 2024

Fix float multiplication that results in subnormals (or zero), and enable tests for those cases. Also remove dead code left over from when this was ported from compiler-rt (the other branches of the if statement are unreachable as shift is always less than bits at this point). The equivalent code in compiler-rt does not have this bug.

Fixes #616

@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Jun 30, 2024

There has to be a bug in either GCC or LLVM's libraries at least on some platforms for f128, I was able to reproduce in C llvm/llvm-project#91840.

The clippy failure is from #634 (comment)

@beetrees beetrees force-pushed the fix-float-mul branch 2 times, most recently from 01a7c0c to 01b0b37 Compare June 30, 2024 22:02
@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Jun 30, 2024

Probably just #![allow(out_of_scope_macro_calls)] to fix the rest until rust-lang/rust#126987 gets merged

@beetrees
Copy link
Copy Markdown
Contributor Author

beetrees commented Jun 30, 2024

Allowing the lint results in the contradictory

 error: unknown lint: `out_of_scope_macro_calls`
  --> src/probestack.rs:52:10
   |
52 | #![allow(out_of_scope_macro_calls)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unknown-lints` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unknown_lints)]`

error: cannot find macro `define_rust_probestack` in this scope
   --> src/probestack.rs:137:5
    |
137 |     define_rust_probestack!(
    |     ^^^^^^^^^^^^^^^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #124535 <https://github.com/rust-lang/rust/issues/124535>
    = help: import `macro_rules` with `use` to make it callable above its definition
    = note: `-D out-of-scope-macro-calls` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(out_of_scope_macro_calls)]`

so I've added a temporary use define_rust_probestack; instead.

@beetrees
Copy link
Copy Markdown
Contributor Author

beetrees commented Jun 30, 2024

CI is now green 🎉. Thanks for the help.

tgross35
tgross35 reviewed Jul 1, 2024
View reviewed changes
Comment thread testcrate/src/bench.rs
Comment on lines -20 to +23
// FIXME(f16_f128): rounding error
// FIXME(#616): re-enable once fix is in nightly
// <https://github.com/rust-lang/compiler-builtins/issues/616>
"mul_f128",
"mul_f32",
"mul_f64",
Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why will these be fixed with the nightly bump? I guess if __mul[sd]f3 don't exist since they are expected to lower to assembly, then linking the extern "C" symbols is linking to the weak ones from this library?

Copy link
Copy Markdown
Contributor Author

@beetrees beetrees Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your guess is correct.

tgross35
tgross35 reviewed Jul 1, 2024
View reviewed changes
Comment thread testcrate/src/bench.rs Outdated
Comment on lines +36 to +39
// FIXME(f16_f128): Incorrect rounding.
// <https://github.com/llvm/llvm-project/issues/91840>
const AARCH64_SKIPPED: &[&str] = &["mul_f128"];
Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this must have been reproduced in CI then, which is good. Any idea where the problem is coming from? https://github.com/llvm/llvm-project/blob/8598bcb9934dca16ea16d87304e00defc85d986c/compiler-rt/lib/builtins/fp_lib.h and https://github.com/llvm/llvm-project/blob/8598bcb9934dca16ea16d87304e00defc85d986c/compiler-rt/lib/builtins/fp_mul_impl.inc don't seem to have any architecture-specific behavior except for fmaxf.

Copy link
Copy Markdown
Contributor Author

@beetrees beetrees Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just reproduced the bug using the instructions from llvm/llvm-project#91840 on x86_64-unknown-linux-gnu; I don't think the bug is platform specific, just compiler-rt specific. It only shows up on AArch64 in CI as that's the only architecture tested in compiler-builtins CI that used to build the C version of the builtin in the version of compiler-builtins currently used by nightly rustc (in the build.rs that's currently used by nightly, notice how multf3.c is only included on aarch64/arm64ec, mips64 and loongarch64).

Copy link
Copy Markdown
Contributor Author

@beetrees beetrees Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the FIXME.

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great, guess I just didn't test this on other platforms. Thanks for testing

@Amanieu Amanieu merged commit c1c8fcc into rust-lang:master Jul 5, 2024
@beetrees beetrees deleted the fix-float-mul branch July 5, 2024 21:27
tgross35 pushed a commit that referenced this pull request Jan 31, 2026
@Amanieu
Merge pull request #637 from beetrees/fix-float-mul
748ab3f 
Fix incorrect rounding with subnormal/zero results of float multiplication
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgross35 tgross35 tgross35 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rounding error in __mulsf3

3 participants

@beetrees @tgross35 @Amanieu